packages/node-core/src/indexer/indexer.manager.ts (1)

140-149: Consider a more robust comparison for args.

The current implementation uses JSON.stringify to compare datasource arguments (lines 144-145), which can produce false negatives if object keys are in different orders. Additionally, the fallback chain for retrieving args from processor.options || options is fragile and may not correctly locate args across different datasource types.

Consider using a deep-equality utility (e.g., lodash's isEqual) or normalizing the comparison by sorting object keys before stringification.

Apply this approach:

+import {isEqual} from 'lodash';

  // Filter out the destroyed datasource by matching startBlock and args
  // Note: Reassigning filteredDataSources is intentional - subsequent handlers
  // within the same block will see the updated filtered list
  filteredDataSources = filteredDataSources.filter((fds) => {
    const fdsStartBlock = (fds as any).startBlock;
    // For custom datasources, args are stored in processor.options
    // For runtime datasources, they may be stored differently
-   const fdsArgs = JSON.stringify((fds as any).processor?.options || (fds as any).options || {});
-   const paramArgs = JSON.stringify(destroyedDsParam.args || {});
+   const fdsArgs = (fds as any).processor?.options || (fds as any).options || {};
+   const paramArgs = destroyedDsParam.args || {};

    // Keep datasource if it doesn't match the destroyed one
-   return !(fdsStartBlock === destroyedDsParam.startBlock && fdsArgs === paramArgs);
+   return !(fdsStartBlock === destroyedDsParam.startBlock && isEqual(fdsArgs, paramArgs));
  });

packages/node-core/src/indexer/dynamic-ds.service.ts (1)

137-183: Consider type-safe endBlock assignment.

The destruction logic is comprehensive with good validation. However, line 178 uses as any to assign endBlock to the datasource object, which bypasses TypeScript's type safety.

If the datasource type doesn't include endBlock, consider:

1. Updating the base datasource types to include an optional endBlock property, or
2. Documenting why the type assertion is necessary (e.g., if datasource types are dynamically extended at runtime)

This would improve type safety and make the code's intent clearer.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/node-core/src/indexer/indexer.manager.ts (1)

119-123: Optional: return immutable copies from getDynamicDatasources.

Handlers could accidentally mutate returned info objects. Consider returning Object.freeze()-d copies to harden the VM API. No behavior change required.

packages/node-core/src/indexer/dynamic-ds.service.spec.ts (1)

262-333: Add tests for identical startBlock with different args + param lookup.

• Add a case with two 'Test' datasources sharing startBlock but different args; destroy by global index should affect only the targeted one.
• Add a small unit for getDatasourceParamByIndex (valid index, OOB).

Example:

it('destroys by global index when startBlock collides but args differ', async () => {
  const a = {templateName:'Test', startBlock: 10, args:{id: 'A'}};
  const b = {templateName:'Test', startBlock: 10, args:{id: 'B'}};
  const meta = mockMetadata([a, b]);
  await service.init(meta);
  await service.destroyDynamicDatasource('Test', 20, 1); // destroy 'B'
  const params = (service as any)._datasourceParams;
  expect(params[0]).toEqual(a);
  expect(params[1]).toEqual({...b, endBlock: 20});
});

it('getDatasourceParamByIndex returns undefined when OOB', async () => {
  const meta = mockMetadata([testParam1]);
  await service.init(meta);
  expect(service.getDatasourceParamByIndex(5)).toBeUndefined();
  expect(service.getDatasourceParamByIndex(0)).toEqual(testParam1);
});

packages/node-core/src/indexer/dynamic-ds.service.ts (1)

134-181: Add a guard to prevent destruction before startBlock.

Defensive check avoids setting endBlock earlier than the datasource starts.

Apply this diff:

   // Validate it matches the template name and is not already destroyed
   if (dsParam.templateName !== templateName) {
     throw new Error(
       `Datasource at index ${index} has template name "${dsParam.templateName}", not "${templateName}"`
     );
   }

   if (dsParam.endBlock !== undefined) {
     throw new Error(`Dynamic datasource at index ${index} is already destroyed`);
   }
+
+  // Prevent destroying before the datasource has started
+  if (currentBlockHeight < dsParam.startBlock) {
+    throw new Error(
+      `Cannot destroy datasource at index ${index} before its startBlock (${dsParam.startBlock})`
+    );
+  }

Optionally, expose a helper for the manager to remove items in-place without array scans:

+  /** Returns the DS object by global index (active or destroyed). */
+  getDatasourceByIndex(index: number): DS | undefined {
+    return this._datasources?.[index];
+  }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/node-core/src/indexer/indexer.manager.ts (1)

124-143: Confirmed: The matching logic is redundant and fragile. Use object reference tracking instead.

The review comment correctly identifies a real issue. The DynamicDsService maintains synchronized _datasources and _datasourceParams arrays indexed by global index. When destroyDynamicDatasource(templateName, blockHeight, index) is called, the index parameter directly identifies the datasource via this._datasourceParams[index].

The current code at lines 132–138 unnecessarily re-matches datasources in filteredDataSources using property comparison (startBlock + serialized options/args), which:

1. Is redundant: The index already identifies the exact datasource
2. Fails with property mismatch: Compares (fds as any).options || (fds as any).processor?.options against args — these are distinct properties with unclear relationship
3. Is brittle: Multiple datasources could share the same startBlock and args; JSON.stringify is order-dependent

Solution: Store the created datasource reference when adding to filteredDataSources, then use it for removal:

        // Inject function to create ds into vm
        vm.freeze(async (templateName: string, args?: Record<string, unknown>) => {
          const newDs = await this.dynamicDsService.createDynamicDatasource({
            templateName,
            args,
            startBlock: blockHeight,
          });
          // Push the newly created dynamic ds to be processed this block on any future extrinsics/events
          filteredDataSources.push(newDs);
          dynamicDsCreated = true;
        }, 'createDynamicDatasource');

        // Inject function to destroy ds into vm
-       vm.freeze(async (templateName: string, index: number) => {
+       const createdDsMap = new Map<number, DS>();
+       vm.freeze(async (templateName: string, index: number) => {
          await this.dynamicDsService.destroyDynamicDatasource(templateName, blockHeight, index);

-         // Remove the destroyed datasource from the current processing array
-         // Find the datasource by matching the global index stored in the service
-         const destroyedDsParam = this.dynamicDsService.getDatasourceParamByIndex(index);
-         if (destroyedDsParam) {
-           const dsIndex = filteredDataSources.findIndex((fds) => {
-             return (
-               fds.startBlock === destroyedDsParam.startBlock &&
-               JSON.stringify((fds as any).options || (fds as any).processor?.options || {}) ===
-                 JSON.stringify(destroyedDsParam.args || {})
-             );
-           });
-           if (dsIndex !== -1) {
-             filteredDataSources.splice(dsIndex, 1);
-           }
-         }
+         // Remove by reference (stored during creation)
+         const destroyed = createdDsMap.get(index);
+         if (destroyed) {
+           const pos = filteredDataSources.indexOf(destroyed);
+           if (pos !== -1) filteredDataSources.splice(pos, 1);
+         }
        }, 'destroyDynamicDatasource');

        // Store reference mapping during creation
-       vm.freeze(async (templateName: string, args?: Record<string, unknown>) => {
+       const originalCreate = async (templateName: string, args?: Record<string, unknown>) => {
          const newDs = await this.dynamicDsService.createDynamicDatasource({
            templateName,
            args,
            startBlock: blockHeight,
          });
          filteredDataSources.push(newDs);
+         const currentIndex = this.dynamicDsService.dynamicDatasources.length - 1;
+         createdDsMap.set(currentIndex, newDs);
          dynamicDsCreated = true;
-       }, 'createDynamicDatasource');
+       };
+       vm.freeze(originalCreate, 'createDynamicDatasource');

packages/node-core/src/indexer/dynamic-ds.service.ts (1)

18-23: Consider adding JSDoc for the endBlock field.

While the implementation is correct, adding documentation would help users understand when and how endBlock is set.

 export interface DatasourceParams {
   templateName: string;
   args?: Record<string, unknown>;
   startBlock: number;
+  /** Block height where this datasource stops processing (set when destroyed) */
   endBlock?: number;
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

packages/node-core/src/indexer/indexer.manager.ts (1)

124-129: Destroyed datasource still processes remaining handlers in the same block.

Per the past review discussion with @stwiname, setting endBlock alone doesn't prevent the destroyed datasource from processing remaining handlers within the same block. The filterDataSources condition at line 154 uses >= nextProcessingHeight, so a datasource with endBlock === blockHeight is still included.

The comment indicates child classes should check ds.endBlock, but this requires coordination across all implementations. Consider:

1. Removing the destroyed DS in-place from filteredDataSources using splice() after destruction, or
2. Adding a test that confirms and documents the intended behavior (that destruction takes effect on the next block, not immediately)

#!/bin/bash
# Check if child implementations check endBlock before processing handlers
rg -nC5 'indexBlockData|endBlock' packages/node/src/indexer/ packages/node-core/src/indexer/ --type ts -g '!*.spec.ts' | head -80

packages/node-core/src/indexer/dynamic-ds.service.ts (1)

176-178: Consider adding endBlock to BaseDataSource type for cleaner typing.

The Object.assign workaround is functional but the comment acknowledges the type mismatch. If BaseDataSource in @subql/types-core doesn't include endBlock, consider extending the type or using a union type to avoid runtime property assignment that bypasses TypeScript's type system.

For now, the implementation is correct - just a suggestion for future type safety improvements.

packages/node-core/src/indexer/worker/worker.dynamic-ds.service.ts (1)

42-56: Consider serialization consistency for cross-thread communication.

createDynamicDatasource (line 39) uses JSON.parse(JSON.stringify(params)) for serialization over the worker bridge. The new methods don't apply the same pattern. While this may be fine if the worker thread uses a shared memory bridge or if the data is already serializable, consider whether getDatasourceParamByIndex should also ensure its return value is properly serialized.

If the bridge handles serialization automatically, this is fine as-is.

packages/node/src/indexer/indexer.manager.ts (1)

201-202: Add endBlock property to type definition for better type safety.

The type assertion (ds as { endBlock?: number }) at lines 201-202 bypasses TypeScript type checking. Since endBlock is being dynamically set and accessed in this lifecycle management feature, consider adding endBlock?: number to the SubstrateProjectDs type definition to maintain type safety throughout the codebase. Alternatively, use a type guard function instead of type assertion.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro